-
-
Notifications
You must be signed in to change notification settings - Fork 13
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Vectors for Replay Mode #9
Conversation
# Conflicts: # CMakeLists.txt # src/common.cpp # src/common.hpp # src/eval_mesh.py # src/partition_mesh.py # src/preciceMap.cpp
# Conflicts: # src/eval_mesh.py
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The overall direction of the work looks good but the implementation needs some polish.
It is a bit unclear to me how to handle the user functions in the scripts. We need some more documentation, input sanitization and examples there.
The implementation in the preciceMap needs some refactoring. It can be simplified and reduced in many corners.
Important is that we are careful regarding the preCICE interface calls. ASTE is used for benchmarking some internals, so the runtime should not be overly affected by input sanitization.
Looking forward to your next request! 👍
src/partition_mesh.py
Outdated
@@ -299,6 +297,7 @@ def parse_args(): | |||
parser.add_argument("--log", "-l", dest="logging", default="INFO", | |||
choices=["DEBUG", "INFO", "WARNING", "ERROR", "CRITICAL"], | |||
help="Set the log level. Default is INFO") | |||
parser.add_argument("--datadim", "-d", dest="datadim", default=1, type=int, help="Dimensions of the function. Default is 1 (Scalar function.") |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
At this point it is simply the dimension of the data.
src/eval_mesh.py
Outdated
@@ -46,6 +47,8 @@ def parse_args(): | |||
parser.add_argument("--log", "-l", dest="logging", default="INFO", | |||
choices=["DEBUG", "INFO", "WARNING", "ERROR", "CRITICAL"], help="""Set the log level. | |||
Default is INFO""") | |||
parser.add_argument("--dimf","-d", dest="dimf", default="1", choices=["1","2", "3"], |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This gets a bit tricky now as the function has to return a 1, 2, or 3-dimensional result.
Maybe we could add a verification step to check |f(v)| = dim
. We could use the vertex coordinates of the first vertex in the mesh.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Those changes are leftovers from testing. For the tutorial we can omit that line.
Does anyone use this file except for the Bunny-Bloodcell example?
If you think it is very useful to extend eval_mesh.py
to vectordata I can do that properly, otherwise I would discard the changes here.
src/mesh_io.py
Outdated
if len(parts) == 4: | ||
pointdata.append(float(parts[3])) | ||
if len(parts) == 6: | ||
pointvector = () | ||
for i in range(3): | ||
pointvector += (float(parts[3+i]),) | ||
pointdata.append(pointvector) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Could we simplify this by simply cutting the parts
into the coordinate and the data on the position 3?
x y z u v w
^^^^^...... coordinate
^^^^^ data
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I tried to simpyfy it, however we have to take care of 3 cases anyway: positions only, scalars where pointdata
is a list of float
s and vectors where pointdata
is a list of lists.
src/mesh_io.py
Outdated
@@ -121,17 +138,24 @@ def read_txt(filename): | |||
return points, cells, cell_types, pointdata | |||
|
|||
|
|||
def write_vtk(filename, points, cells = None, cell_types = None, pointdata = None, tag = None): | |||
def write_vtk(filename, points, cells = None, cell_types = None, pointdata = None, tag = "MyVectors", datadim=3): |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is there a reason why we need to set the tag to MyVectors
by default?
src/mesh_io.py
Outdated
if len(pointdata.shape) == 1 or pointdata.shape[1]==1: #scalar data | ||
DataArray.InsertTuple1(i, pointdata[i]) | ||
elif len(pointdata.shape) > 1 and pointdata.shape[1]==2: #two dimensional data | ||
DataArray.InsertTuple2(i, pointdata[i,0], pointdata[i,1]) | ||
elif len(pointdata.shape) > 1 and pointdata.shape[1]==3: #3D-data | ||
DataArray.InsertTuple3(i, pointdata[i,0], pointdata[i,1], pointdata[i,2]) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
len(pointdata.shape) == 1 or pointdata.shape[1]==1
I would assume consistency here.
Maybe we can assert it after reading the file?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Actually pointdata.shape[1]==1
does not make sense for a 1D array.
|
||
for i in range(1,args.n+1): | ||
files.append(args.SolverName +".dt" + str(i) + ".vtk") | ||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Please check if the above all files
actually exist and tell the user which ones are missing.
This makes debugging easier.
src/precice_to_aste.py
Outdated
help="""Name of the participant that exported the data. | ||
Files are named SolverName.dti.vtk""") | ||
parser.add_argument("--ntimesteps", "-n", dest="n", type= int, help="""Number of timesteps""") | ||
parser.add_argument("--log", "-l", dest="logging", default="INFO", |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Are you actually logging anything? 😉
src/preciceMap.cpp
Outdated
std::vector<std::array<double, 3>> positions; | ||
std::vector<std::array<size_t, 2>> edges; | ||
std::vector<std::array<size_t, 3>> triangles; | ||
std::vector<double> data; | ||
}; | ||
|
||
class VectorMesh : public Mesh{ | ||
public: | ||
VectorData data; //Overload data with Vector data |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
An alternative implementation could be to simply use std::vector<double> data
with a size_t dataStride = 1
in Mesh
. This way you just need to call writeBlockScalarData
or writeBlockVectorData
later.
This would save you to go through the trouble of adding an extra class and reduce code duplication.
Remember that this tool should be as minimal and lightweight as possible. (Someone needs to maintain it.)
src/preciceMap.cpp
Outdated
/// Reads the mesh from the file. If not read_data, zeros are returned for data. | ||
Mesh readMesh(const std::string& filename, bool read_data = true) | ||
Mesh readMesh(const std::string& filename, bool read_data = true, bool vectordata = false) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The flag can be deduced from the actual data.
src/preciceMap.cpp
Outdated
size_t i=0; | ||
VectorData initialdata = readVectorData(meshes[0]); | ||
|
||
for(auto const& vertexID : vertexIDs){ |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
As mentioned earlier, we should use the writeBlockVectorData
variant. The non-block version is more expensive as it has to sanitize the user input for each call.
I reviewed the requested changes and hope that the pull request is ready to merge. |
We have to put this PR on hold until we cleaned up the repository state after the latest changes. |
Superseded by #28 |
This PR brings vector functionality to aste that is needed for the Replay Mode FSI tutorial.
The main changes are in
partition_mesh.py
andpreciceMap.cpp
.